review: fix the out/ artifact upload (allowed-paths must match staging-relative paths)#243
Conversation
…live cases Phase 1 of the live A/B eval plan (#232). The corpus gains an opt-in live block carrying what a real model run needs and the deterministic replay does not: - prContext (PR title/description/author/base; the description is untrusted author text, so an adversarial case can carry its payload there or in the diff), - a post-change file tree on disk next to the case, via a new <id>/case.json + <id>/tree/ layout that coexists with flat <id>.json (a directory containing case.json is one case; its tree is never parsed as corpus JSON), and - labeled defect specs (mustCatchSpecs / mustNotFlagSpecs: path, line window, mechanism keyword alternates). Live runs choose their own finding ids, so ground truth matches on anchor window + mechanism rather than id. The loader enforces the invariants: the live tag and the live block imply each other, a live case needs a cleanly-parseable diff, spec paths must appear in changedFiles and the diff, and every non-removed changed file must exist in the tree. loadLiveCorpus() returns the subset. The live half lives in corpus/live.ts; loader.ts re-exports it as the single public surface. Ten cases are converted with hand-authored real diffs and trees: five smoke incidents, both clean cases, one adversarial injection whose payload is a code comment in the diff, one golden holdout, and one synthetic mutation. Their recorded line anchors are rewritten to the authored defect lines (natural files beat content padded to synthetic line numbers), which activates the provenance gate on these cases in the deterministic suite; all expectations hold unchanged.
…action and case staging Phases 2a/2b of the live A/B eval plan (#232), stacked on the Phase 1 corpus format (#233). agent-extract.ts turns review.md's '## agent:' sections into data (name, description, pinned model, prompt body). It takes the markdown as a string with no fs access, because the baseline arm of an A/B reads the merge-base review.md via git show. Parsing is strict; a malformed or model-less section throws listing every problem, since a silently dropped agent would skew an eval arm without failing it. An integration test extracts the real review.md (21 agents) and pins the staging-root reference so a future rename fails a test instead of silently staging nothing. live-stage.ts materializes the production staging layout for one live-enabled corpus case: pr-context.json (review.md Step 1's shape, synthetic identity fields), full.diff / pr.diff / full-stripped.diff (all the case diff; corpus diffs carry no generated files and no pattern-triage pass runs), files.json + review-files.json with the hasPatch cross-check derived from the diff parse, provenance.json, routing.json from the deterministic router, an out/ directory, and the post-change checkout copied from the case tree. rewriteAgentPrompt swaps the production staging root for the staged context dir. Everything sits behind an injected-fs seam and is memfs-tested. Model dispatch (phase 2c) is deliberately absent; it needs the Agent SDK dependency decision and arrives separately.
… runner (phase 2c)
live-producer.ts runs the live roster over one staged case behind an
injected LiveAgentRunner seam (the judge.ts pattern), so its logic is
stub-tested with zero model calls. Roster: the default finders
(correctness-reviewer, skill-auditor) plus the router's lensesToSpawn;
no pattern-triage or thread-reconciler in eval. It maps all three
sub-agent output contracts into the shapes the deterministic runner
consumes: label-shape findings (correctness lens, or conventions for
the skill-auditor so labelForFinding reproduces the best-practice
variants; confidence defaulted to 0.7 per the production claims rule),
structured-schema lens findings validated as-is, and the validator's
{claims: [...]} verdicts into CaseVerification[]. It stages
claims.json for the validator, resolves {{#runtime-import}} directives
against the case tree (a case opts into a skills index by carrying the
file), retries once on malformed output with the rejection fed back,
keeps partial results when an agent fails twice, prefixes colliding
finding ids, and reports per-agent cost/turns/wall-clock.
live-runner.ts is the one module that touches a real model runtime:
an Agent SDK query() per dispatch with Read/Grep/Glob only, cwd pinned
to the staged checkout, the agent's pinned model, and hard turn and
wall-clock caps; plus the CLI smoke entry
(tsx live-runner.ts --case <id>, requires ANTHROPIC_API_KEY). The
investigation-cap CLI the prompts reference is not staged; its own
denied-budget fallback applies. Adds @anthropic-ai/claude-agent-sdk
as a dev dependency.
live-match.ts scores a live run against a case's labeled defect specs: a posted candidate satisfies a spec when its anchor agrees with the spec's path (and line window when both carry one) and any mechanism alternate matches the finding's failure_scenario or prose; each candidate satisfies at most one spec and vice versa. An injected fallback arbiter (hard-capped, same-file only, recorded as via: fallback for audit) can rescue recall on vague prose; false flags are decided by the deterministic rule alone. computeLiveMetrics aggregates recall, verdict agreement, clean false-flag (including a clean case that blocks), and noise. live-ab.ts is the arm orchestrator and CLI: baseline review.md from git show <merge-base>, candidate from the working tree, both arms over the same live corpus with everything else (corpus, lib, runner, metrics, judge) from the candidate, per the plan's settled decision to isolate the model seam. Each arm runs under half the --max-usd budget with sticky exhaustion: once spend plus the running per-case average crosses the cap, remaining cases are recorded skipped and the report still emits. Spec-level regressions are diffed only over cases both arms scored. Judge scoring reuses the pinned judge (quality aggregates only; judge-vs-ground-truth disagreement keys on recorded ids a live arm does not use); the fetch model moves to judge-live-model.ts and live-judge.ts now imports it. runner.ts gains an optional RunOptions.validation override so a live validator's output replaces the recorded block. Report-only except the standing rule: adversarial-injection failures on the candidate arm exit non-zero.
Review Eval A/B runs on every non-draft PR touching workflows/review/** (and on workflow_dispatch): both review.md arms over the live corpus via live-ab.ts, with the delta report posted as a sticky PR comment (hidden-marker upsert), appended to the job summary, and uploaded as an artifact even on partial or gate-failing runs. Per-PR scope is the smoke-tagged live subset; the full-eval label or dispatch input lifts it, skip-live-eval opts out, drafts wait until ready, the changeset-release branch is excluded (it matches the path filter via package.json but changes no behavior), secretless runs skip green so fork PRs never fail, and per-PR concurrency cancels superseded runs. The workflow name is distinct from every gh-aw workflow per the operational-floor rule about shared concurrency groups. live-ab.ts gains --smoke-only and writes out/live-ab-report.md alongside the JSON for the comment step.
… and vitest Tree directories are byte-exact case fixtures paired with each case's diff: prettier auto-formatting one would silently desync it from the diff the provenance gate parses, and a tree may carry a *.test.ts whose tests fail by design (test-adequacy cases), so vitest must not execute them either. CI's lint job caught the first drift (prettier wanting to rewrap a fixture ternary).
…rpus' into jwbron/live-eval-producer-staging
…staging' into jwbron/live-eval-ab-runner
…to jwbron/live-eval-ab-ci
…s with the case id Live agents choose their own finding ids, so every case's first correctness finding was live-correctness-reviewer-1; ids were unique within a case but collided across cases, and judge.ts's score join requires arm-global uniqueness. Caught by the first real A/B run (both arms completed, then judge aggregation threw). Ids are now <caseId>:<id> from the moment of parsing, so claims.json, the validator round-trip, the matcher, and the judge all see the same namespaced id.
…staging' into jwbron/live-eval-ab-runner
…eport instead of killing it The first real A/B run spent both arms' budgets and then died in judge aggregation, writing no report: the exact everything-spent-nothing-posted failure mode the plan forbids. Judge scoring is additive, so a per-arm failure is now caught, recorded as judgeError on the arm, rendered as a degradation note in the report, and the run proceeds to write JSON + markdown and evaluate the adversarial gate as usual.
…to jwbron/live-eval-ab-ci
🦋 Changeset detectedLatest commit: 47fb67e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
63c8e1e to
6a5adf7
Compare
…A/B phase 5) Packages the seeded-defect live-trial pattern (Khan/webapp#40678) as a Claude Code skill so a trial costs an operator an afternoon instead of a week of hand choreography. The skill collects required inputs (seeded branch, human-authored defect table, arms, budget approval) and refuses to improvise ground truth; sets up one isolated PR per arm with per-arm trigger recipes and the distinct-workflow-name rule (same-named gh-aw workflows share a per-PR concurrency group and cancel each other); collects reviews, artifacts, and costs per run with the known gh-aw artifact-bug tolerance; drives optional lifecycle pushes; scores defect by defect with the deterministic rule mirroring eval/live-match.ts plus audited manual judgment; exports live-enabled corpus case skeletons with a sanitization gate for private-repo content landing in this public repo; and cleans up trial PRs, branches, and temporary workflows. Trials remain the architecture-bet instrument; per-change evals belong to the corpus A/B. .gitignore narrows from .claude to .claude/* with a !.claude/skills/ carve-out: local agent state (settings, worktrees) stays untracked, project skills are shared tooling and are committed.
|
Restacked: this PR is now based on main and carries only the fix commit (cherry-picked as 6a5adf7; content unchanged, verified by tree diff). Rationale: the fix is behavior-neutral infra with two downstream consumers (#225's live counters, the A/B runner's claim auditing), so it should merge first and gains nothing from waiting behind the eval stack or from A/B coverage. If you have local state on jwbron/review-out-artifact-upload, reset to origin before continuing. Batch two's behavior PRs (re-review accountability, out-of-lane handoff, dispatch-tax trim) should still base on jwbron/review-trial-skill, which now sits on top of the full A/B chain, so each behavior PR gets a per-push Review Eval A/B report. |
…orpus' into tmp-refresh
…roducer-staging' into tmp-refresh
…b-runner' into tmp-refresh
…b-ci' into tmp-refresh
…oad (allowed-paths must match staging-relative paths)
6a5adf7 to
5dd182b
Compare
|
Restack, final topology (supersedes my earlier comment): this PR is back between the eval stack and the behavior PRs, i.e. main -> #233 -> #234 -> #236 -> #237 -> #238 -> #243 -> #244 -> #245. Two things happened: (1) the whole eval chain had been cut from a stale main that predated review@1.4.0 (missing #228/#230), which would have made every A/B arm and every stacked behavior PR run a pre-v1.4.0 prompt; the chain is now refreshed with main via bottom-up merges (no history rewrites on the chain itself). (2) This branch and the two behavior branches were rebuilt on the refreshed chain (cherry-picks 5dd182b / 491a983 / 7b5318c; content unchanged). If you have local state on any of the three batch-two branches, reset to origin before continuing. The dispatch-tax trim PR should base on jwbron/review-out-of-lane. |
Review live A/BBaseline:
Regressions (baseline caught, candidate missed)
Adversarial hard gate: PASSED on the candidate arm. Agent failures
Single-run-stable rows: recall, verdict agreement, regressions, adversarial gate. Judge quality and noise are not: they jitter run-to-run at this corpus size, and a regressed reviewer can score HIGHER on judge quality (fewer, surer comments each read better). Recall against the labeled specs is the load-bearing metric. |
…p retry, drop-bucket taxonomy Three instrument fixes from the eval-tuning memo, landed in the runner so the whole stack above inherits them: - Pre-flight identity short-circuit (memo item 1): byte-identical review.md in both arms posts a no-reviewable-delta report and runs nothing; --force-arms preserved for deliberate wobble controls. - Gate-flip retry (memo item 3): an adversarial hard-gate flip re-runs only the flipped cases, best of three, before the gate may fail the arm. Retried runs never replace the original in the metrics; they only decide whether the flip was a run-to-run flake. Retry spend is recorded in the report. - Drop-bucket taxonomy (memo item 4): each missed must-catch spec is classified true-miss vs found-but-dropped (provenance/scope/ validation) by re-matching the spec against the dropped candidates with the line window relaxed (a mis-anchored real finding is exactly the case to surface). The report annotates every lost regression with its class and carries a found-but-dropped count row.
…y with backoff) The judge grades one comment's prose in 512 tokens; every load-bearing metric (recall, verdict agreement, regressions, adversarial gate) is deterministic and never touches it, and the acceptance runs showed the judge signal is not single-run-stable on any model (it moved 0.11 on the byte-identical control and went UP on the weakened arm). Price that signal accordingly: the pin moves from claude-opus-4-8 to claude-haiku-4-5-20251001 (a fifth of the cost; dated snapshot so week-over-week scores compare; both arms always share one judge, so within-run deltas are unaffected). Supersedes operator direction 4, which pinned Opus. Also adds retry with backoff (3 attempts, 429/408/ 5xx/network only) to the one live judge seam; a single transient 500 previously wasted an entire weekly scoring pass.
…e tip, not the merge-base The workflow passes origin/<base_ref> (the base branch tip) while its comments and the dispatch-input description said merge-base. The code is the right side of that disagreement: pull_request runs check out the PR merge commit, so the candidate tree already contains the base tip; baselining on the same tip isolates the PR's own review.md delta, where a merge-base baseline would fold upstream review.md movement into the PR's measured numbers. Prose updated to match the code; no behavior change.
…to jwbron/live-eval-ab-ci
…o jwbron/review-trial-skill
…-skill' into jwbron/review-out-artifact-upload
…fetch signature (eslint no-undef)
…to jwbron/live-eval-ab-ci
…o jwbron/review-trial-skill
…-skill' into jwbron/review-out-artifact-upload
Review Guidancegithub-actions (1 file)
Excluded from review (1 file)Not individually deep-reviewed (whole-change reviewers still saw it):
|
| retention-days: 30 | ||
| allowed-paths: | ||
| - "/tmp/gh-aw/review/out/**" | ||
| - "out/**" # staging-relative layout (what v0.81.6 matches) |
There was a problem hiding this comment.
thought (non-blocking): Under staging-relative matching, out/** matches any directory named out regardless of where it lives, so allowed-paths no longer constrains the upload's source location — the containment the absolute pattern used to express can't be restored at this layer (a gh-aw v0.81.6 limitation). Worth a one-line note here, and an upstream gh-aw issue, so the old guarantee isn't implied.
| allowed-paths: | ||
| - "/tmp/gh-aw/review/out/**" | ||
| - "out/**" # staging-relative layout (what v0.81.6 matches) | ||
| - "/tmp/gh-aw/review/out/**" # original absolute path (future-proofing) |
There was a problem hiding this comment.
question (non-blocking): Given the comment states this absolute pattern matches nothing under v0.81.6, is it worth keeping? gh-aw is version-pinned, so a matching-semantics change would arrive via an explicit bump — the natural point to revisit this line. Keeping a self-described dead pattern is defensible as future-proofing, but it does make the config contradict its own comment.
| "review": patch | ||
| --- | ||
|
|
||
| Fix the out/ run-artifact upload, which failed on every run under gh-aw v0.81.6. The orchestrator passed the absolute path `/tmp/gh-aw/review/out/` exactly as instructed, but gh-aw's upload_artifact tool stages an uploaded directory under its basename and records only the staging-relative name (`out`), and the safe_outputs job then filters the staged files (`out/<agent>.json`) against `allowed-paths` with a fully anchored matcher; the absolute pattern `/tmp/gh-aw/review/out/**` therefore matched nothing, every run annotated `ERR_VALIDATION: upload_artifact: no files matched the selection criteria`, and no sub-agent outputs or `pre-existing.json` reached the run artifacts, blocking post-hoc claim auditing, the live counters, and the A/B runner, which all read them. `allowed-paths` now lists the staging-relative `out/**` (plus the absolute form, since the filter is an OR, in case a future gh-aw matches original paths). Step 9 also copies `claims.json` into `out/` before uploading, so the artifact carries the validator's input alongside its verdicts (`claim-validator.json`) and the provenance gate's set-asides (`pre-existing.json`). |
There was a problem hiding this comment.
nitpick (non-blocking): The five sibling changesets open with a short noun-phrase label plus a parenthetical plan anchor (e.g. (live A/B plan, phase 3)); this one opens straight into the fix narrative. A leading label/anchor would keep the generated CHANGELOG consistent.
# Conflicts: # workflows/review/eval/corpus/live.ts
Summary
First of five PRs in the fold-in batch two stack, stacked on #238 (
jwbron/review-trial-skill, the top of the open eval stack). Stack order: this PR, then re-review accountability, out-of-lane handoff, dispatch-tax trim, and quoted-rule rendering. The top of the batch-two stack will bejwbron/review-skill-rule-quote; downstream work (the re-review mode dial, the webapp preview runs) should base on that branch.On gh-aw v0.81.6 the safe_outputs job of every review run annotates
ERR_VALIDATION: upload_artifact: no files matched the selection criteria, and none of the out/ files (sub-agent outputs,pre-existing.json) reach the run artifacts; that blocks post-hoc claim auditing and everything that reads run artifacts (the #225 live counters, the A/B runner).Root cause (traced through the gh-aw v0.81.6 source and the run logs of the v1.4.0 re-run lifecycle, runs 29044383251 / 29045581242 / 29046928431 of
review-preview.lock.ymlon Khan/webapp#40730):upload_artifactcalled with{"path": "/tmp/gh-aw/review/out/"}.safe_outputs_handlers.cjsuploadArtifactHandler) copies the directory into its staging area under the directory's basename and rewrites the recorded path to the staging-relativeout(the JSONL entry is{"type":"upload_artifact","path":"out"}).upload_artifact.cjsresolveFiles) lists the staged files relative to the staging dir (out/<agent>.json) and filters them againstallowed-pathswith a fully anchored regex (glob_pattern_helpers.cjs). A staging-relative path can never match the absolute pattern/tmp/gh-aw/review/out/**, so the filter empties the candidate set on every run and the handler returns the observed ERR_VALIDATION. With an absolute pattern, no tool input can succeed; upstream main (checked at f2bff5d5) has the same behavior.The fix is config-side:
allowed-pathsnow lists the staging-relativeout/**, keeping the absolute form alongside it (the filter is an OR across patterns) in case a future gh-aw release matches against the original path. The Step 9 prompt keeps the absolute-directory-path instruction (it is correct) but now explains the actual staging semantics instead of the outdated rationale, and adds one step: copyclaims.jsonintoout/before uploading, so the artifact carries the validator's input alongside its verdicts (claim-validator.json) and the provenance gate's set-asides (pre-existing.json), completing the claim-audit trail.Failure scenario fixed: a reviewer investigating why run 2 of the lifecycle left three blocking threads unacknowledged has no
out/thread-reconciler.jsonto consult; the #225 live counters find no artifact and report nothing; the A/B runner cannot score producer outputs. After this change theoutartifact uploads with all staged files.Note for reviewers: this branch was rebuilt on the refreshed eval chain (see the restack/topology comments below); the fix now rides a single cherry-picked commit on top of #238's branch, content unchanged from the original.
One adjacent gap surfaced while verifying, not fixed here: the shipped review-v1.4.0 tag still pins the
pre-agent-stepslib checkout atref: review-v1.2.2(review.md line 208), which predateslib/provenance.ts; a consumer compiling plain review.md@review-v1.4.0 gets a lib checkout that cannot run the provenance CLI (the gate fails open with a note). The webapp preview lock pins a newer ref, which is why the lifecycle runs were unaffected. Worth folding a ref bump into the release process rather than hand-bumping in this stack.cc @jeresig
Testing
npx vitest run workflows/review: 457 tests green.globPatternToRegex("/tmp/gh-aw/review/out/**")rejectsout/claim-validator.json,out/pre-existing.json,out/claims.json;globPatternToRegex("out/**")matches all three.safe-outputs-upload-artifactshandoff artifact of run 29046928431 contains all 11out/*.jsonfiles), so the only failure is the pattern filter this PR changes. An end-to-end run rides the webapp preview recompile that the downstream preview-runs work performs against this stack.